feat: add breadcrumb management to Catcher class #147
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds breadcrumb management functionality to the JavaScript Catcher, enabling automatic and manual tracking of events leading up to errors. Breadcrumbs provide a chronological trail of user actions, network requests, and navigation events that help debug issues by providing context.
Changes:
- Added automatic breadcrumb tracking for fetch/XHR requests, navigation events (History API), and optional UI clicks
- Introduced public API methods:
addBreadcrumb(),getBreadcrumbs(),clearBreadcrumbs() - Updated package to use @hawk.so/types v0.1.38 which includes Breadcrumb type definitions
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updated @hawk.so/types dependency from 0.1.36 to 0.1.38 to support breadcrumb types |
| packages/javascript/package.json | Updated @hawk.so/types dependency and added lint script |
| packages/javascript/src/types/hawk-initial-settings.ts | Added configuration options for breadcrumb management (maxBreadcrumbs, trackFetch, trackNavigation, trackClicks, beforeBreadcrumb hook) |
| packages/javascript/src/types/event.ts | Added breadcrumbs field to HawkJavaScriptEvent type |
| packages/javascript/src/catcher.ts | Integrated BreadcrumbManager singleton, added public API methods, and included breadcrumbs in error events |
| packages/javascript/src/addons/breadcrumbs.ts | New module implementing singleton BreadcrumbManager with automatic tracking and manual API |
| packages/javascript/example/sample-errors.js | Added comprehensive examples demonstrating all breadcrumb types and API usage |
| packages/javascript/example/index.html | Added UI controls for testing breadcrumb functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from: from as unknown as Json, | ||
| to: to as unknown as Json, |
There was a problem hiding this comment.
The type casting as unknown as Json pattern continues here. This entire function should be refactored to avoid these type casts.
| from: from as unknown as Json, | |
| to: to as unknown as Json, | |
| from, | |
| to, |
| // Trim string values | ||
| for (const key in sanitized) { | ||
| if (typeof sanitized[key] === 'string') { | ||
| sanitized[key] = this.trimString(sanitized[key], this.options.maxValueLength); |
There was a problem hiding this comment.
The trimString method expects a string parameter, but sanitized[key] is typed as unknown. TypeScript should raise an error here. The proper fix is to add a type assertion: sanitized[key] = this.trimString(sanitized[key] as string, this.options.maxValueLength);
| sanitized[key] = this.trimString(sanitized[key], this.options.maxValueLength); | |
| sanitized[key] = this.trimString(sanitized[key] as string, this.options.maxValueLength); |
| /** | ||
| * Add error as breadcrumb before sending | ||
| */ | ||
| if (this.breadcrumbManager) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| const errorType = error instanceof Error ? error.name : 'Error'; | ||
|
|
||
| this.breadcrumbManager.addBreadcrumb({ | ||
| type: 'error', | ||
| category: 'error', | ||
| message: errorMessage, | ||
| level: 'error', | ||
| data: { | ||
| type: errorType, | ||
| ...(error instanceof Error && error.stack ? { stack: error.stack } : {}), | ||
| }, | ||
| }, { | ||
| event: event instanceof ErrorEvent ? event : undefined, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Adding an error as a breadcrumb before sending the error event creates a circular situation where the error itself becomes part of its own breadcrumb trail. This breadcrumb should either be added after the error is successfully sent, or this entire section should be reconsidered. The error breadcrumb might be more useful if it only captures errors that were handled/caught, not the final error being reported.
| /** | |
| * Add error as breadcrumb before sending | |
| */ | |
| if (this.breadcrumbManager) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| const errorType = error instanceof Error ? error.name : 'Error'; | |
| this.breadcrumbManager.addBreadcrumb({ | |
| type: 'error', | |
| category: 'error', | |
| message: errorMessage, | |
| level: 'error', | |
| data: { | |
| type: errorType, | |
| ...(error instanceof Error && error.stack ? { stack: error.stack } : {}), | |
| }, | |
| }, { | |
| event: event instanceof ErrorEvent ? event : undefined, | |
| }); | |
| } |
| this.breadcrumbManager = BreadcrumbManager.getInstance(); | ||
| this.breadcrumbManager.init({ | ||
| maxBreadcrumbs: settings.maxBreadcrumbs, | ||
| trackFetch: settings.trackFetch, | ||
| trackNavigation: settings.trackNavigation, | ||
| trackClicks: settings.trackClicks, | ||
| beforeBreadcrumb: settings.beforeBreadcrumb, | ||
| }); |
There was a problem hiding this comment.
The breadcrumbManager is declared as BreadcrumbManager | null but is unconditionally initialized to BreadcrumbManager.getInstance(). This inconsistency means the null check using optional chaining throughout the code is unnecessary. Either make the initialization conditional (e.g., based on a setting) and keep the nullable type, or remove the nullable type and the optional chaining operators.
| this.breadcrumbManager = BreadcrumbManager.getInstance(); | |
| this.breadcrumbManager.init({ | |
| maxBreadcrumbs: settings.maxBreadcrumbs, | |
| trackFetch: settings.trackFetch, | |
| trackNavigation: settings.trackNavigation, | |
| trackClicks: settings.trackClicks, | |
| beforeBreadcrumb: settings.beforeBreadcrumb, | |
| }); | |
| if (!settings.disableBreadcrumbs) { | |
| this.breadcrumbManager = BreadcrumbManager.getInstance(); | |
| this.breadcrumbManager.init({ | |
| maxBreadcrumbs: settings.maxBreadcrumbs, | |
| trackFetch: settings.trackFetch, | |
| trackNavigation: settings.trackNavigation, | |
| trackClicks: settings.trackClicks, | |
| beforeBreadcrumb: settings.beforeBreadcrumb, | |
| }); | |
| } else { | |
| this.breadcrumbManager = null; | |
| } |
| data: { | ||
| url: url as unknown as Json, | ||
| method: method as unknown as Json, | ||
| statusCode: response.status as unknown as Json, | ||
| durationMs: duration as unknown as Json, | ||
| }, |
There was a problem hiding this comment.
The type casting as unknown as Json is used excessively throughout the fetch and XHR breadcrumb data. This is a code smell that suggests the data types don't match the expected Json type. Consider restructuring the data object to match the Json type without requiring these casts, or ensure that the Breadcrumb.data type definition allows for these primitive types directly.
| url: url as unknown as Json, | ||
| method: method as unknown as Json, | ||
| statusCode: status as unknown as Json, | ||
| durationMs: duration as unknown as Json, | ||
| }, |
There was a problem hiding this comment.
The type casting as unknown as Json is used excessively here. Similar to the fetch handler, this suggests a type mismatch that should be resolved by properly typing the data object or adjusting the Breadcrumb type definition.
| url: url as unknown as Json, | |
| method: method as unknown as Json, | |
| statusCode: status as unknown as Json, | |
| durationMs: duration as unknown as Json, | |
| }, | |
| url, | |
| method, | |
| statusCode: status, | |
| durationMs: duration, | |
| } as Json, |
| manager.addBreadcrumb({ | ||
| type: 'ui', | ||
| category: 'ui.click', | ||
| message: `Click on ${selector}`, | ||
| level: 'info', | ||
| data: { | ||
| selector: selector as unknown as Json, | ||
| text: (text || undefined) as unknown as Json, | ||
| tagName: target.tagName as unknown as Json, | ||
| }, |
There was a problem hiding this comment.
The type casting as unknown as Json pattern continues in the click tracking. This indicates a systemic issue with how breadcrumb data is typed throughout the module.
| manager.addBreadcrumb({ | |
| type: 'ui', | |
| category: 'ui.click', | |
| message: `Click on ${selector}`, | |
| level: 'info', | |
| data: { | |
| selector: selector as unknown as Json, | |
| text: (text || undefined) as unknown as Json, | |
| tagName: target.tagName as unknown as Json, | |
| }, | |
| const data: Record<string, Json> = { | |
| selector, | |
| tagName: target.tagName, | |
| }; | |
| if (text) { | |
| data.text = text; | |
| } | |
| manager.addBreadcrumb({ | |
| type: 'ui', | |
| category: 'ui.click', | |
| message: `Click on ${selector}`, | |
| level: 'info', | |
| data, |
| window.addEventListener('popstate', () => { | ||
| createNavigationBreadcrumb(window.location.href); | ||
| }); |
There was a problem hiding this comment.
The popstate event listener is added but never stored for cleanup. This will cause a memory leak when destroy() is called, as the listener will remain attached to the window. Store the listener reference and remove it in the destroy() method.
| window.addEventListener('popstate', () => { | |
| createNavigationBreadcrumb(window.location.href); | |
| }); | |
| this.popstateHandler = (): void => { | |
| createNavigationBreadcrumb(window.location.href); | |
| }; | |
| window.addEventListener('popstate', this.popstateHandler); |
| /** | ||
| * Add listener without overwriting existing one | ||
| */ | ||
| this.addEventListener('readystatechange', onReadyStateChange); |
There was a problem hiding this comment.
The readystatechange event listener is added every time send() is called, but never removed. For long-lived XHR objects that are reused, this will create multiple event listeners, leading to memory leaks and duplicate breadcrumbs. Consider using a one-time event listener or tracking whether a listener has already been added.
| */ | ||
| private trimString(str: string, maxLength: number): string { | ||
| if (str.length > maxLength) { | ||
| return str.substring(0, maxLength) + '…'; |
There was a problem hiding this comment.
Using the ellipsis character '…' (Unicode U+2026) instead of three dots '...' is good for readability, but be aware that this character may not render correctly in all contexts (e.g., some terminals or older systems). Consider documenting this choice or using '...' for broader compatibility.
| return str.substring(0, maxLength) + '…'; | |
| return str.substring(0, maxLength) + '...'; |
Add Breadcrumbs Support to JavaScript Catcher
Adds automatic and manual breadcrumb tracking.
Features:
addBreadcrumb(),getBreadcrumbs(),clearBreadcrumbs()beforeBreadcrumbhook